-
Notifications
You must be signed in to change notification settings - Fork 8
Add support for long xattr prefix #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for long xattr prefixes to the EROFS filesystem implementation, which was previously unimplemented and would return an error. The changes include reading and caching long prefixes from the filesystem metadata and updating the xattr parsing logic to handle them.
Key changes:
- Replaces TODO error returns with actual long prefix support in xattr parsing
- Adds caching mechanism for long prefixes with lazy loading from filesystem metadata
- Includes comprehensive test coverage for files with long xattr prefixes
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| xattr.go | Replaces unimplemented long prefix errors with calls to retrieve cached long prefixes |
| erofs.go | Adds long prefix loading and caching infrastructure with detailed parsing logic |
| erofs_test.go | Adds test cases for files with long xattr prefixes to verify the new functionality |
| README.md | Updates documentation to reflect completed long xattr prefix support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
4d7c8d7 to
2b43857
Compare
|
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Let's just merge as-is, I need to refactor some part e.g. directory data read should go through regular file read (rathar than directly read on-disk original data), since directories are special files and they support compression too |
|
Also, I'm not sure if we'd like to keep test cases in erofs binaries, since previous xz backdoor just leverages random binary data in the source code, it would be helpful to get erofs from scratch. Btw, this commit needs rebasing so that it can be merged. |
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
2b43857 to
91f6c2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@hsiangkao sorry, I didn't push the version with the update before, it is updated now using the new read logic |
hsiangkao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's merge it as-is for now.
Updates testdata to include long prefixes and related tests. In first commit tests are added and fails. The second commit adds support for the prefixes, reading the entire set and caching the prefixes.